x64: convert all multiplication-related instructions #10782
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abrown
commented
May 15, 2025
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "cranelift:meta", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
Implicit operands are used by an instruction but not present in its disassembled output. Instructions like `mul`, e.g., will write to the `%rax` and `%rdx` registers, but this is all invisible in disassembly. Implicit operands are always fixed (i.e., the register is known), but not all fixed operands are implicit (i.e., some fixed registers _are_ disassembled).
…RetValueRegs` Certain `mul*` instructions write to multiple registers. For register allocation, Cranelift needs to know about all of these registers. This change uses the pre-existing pattern of returning a `ValueRegs` type to indicate this. This change is limited to what is needed now: the only multi-return needed now uses two fixed registers.
This does not include any special "small immediate resizing" rules for Winch, so the Winch disassembly tests gain a few bytes (e.g., some immediates that _could_ fit in 8 bits are emitted as the full 32 bits).
abrown
requested review from
cfallin and
alexcrichton
and removed request for
a team
alexcrichton
approved these changes
May 19, 2025
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
May 20, 2025
This commit fixes a minor regression from bytecodealliance#10782 found via fuzzing. The regression is 16-bit immediates were forced to fit from an `i32` value into a `u16` for 16-bit multiplication. This meant though that negative numbers failed this conversion which meant that ISLE would panic due to the value not being matched. This fixes the logic to first fit the i32 into an i16 and then cast that to a u16 where the first phase should hit all the constants that are possible in Cranelift.
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
May 20, 2025
This commit fixes a bug in the new assembler which was surfaced through the changes in bytecodealliance#10782 but was a pre-existing issue. Specifically the encoding of a RIP-relative addressing mode required knowing the number of bytes at the end of an instruction but this was accidentally hardcoded to 0. In bytecodealliance#10782 `imul` instructions were added where a RIP-relative address mode can be used in conjunction with an immediate which cause the RIP-relative addressing to load from the wrong address. This bug can in theory affect other instructions in the new assembler as well, but auditing the list of instructions it looks like `imul` is the only one that can possibly have an immediate after a RIP-relative addressing mode. That means that prior instructions using the new assembler should not be affected.
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
May 20, 2025
This commit fixes a bug in the new assembler which was surfaced through the changes in bytecodealliance#10782 but was a pre-existing issue. Specifically the encoding of a RIP-relative addressing mode required knowing the number of bytes at the end of an instruction but this was accidentally hardcoded to 0. In bytecodealliance#10782 `imul` instructions were added where a RIP-relative address mode can be used in conjunction with an immediate which cause the RIP-relative addressing to load from the wrong address. This bug can in theory affect other instructions in the new assembler as well, but auditing the list of instructions it looks like `imul` is the only one that can possibly have an immediate after a RIP-relative addressing mode. That means that prior instructions using the new assembler should not be affected.
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 20, 2025
* x64: Fix panic compiling 16-bit multiply-with-immediate This commit fixes a minor regression from #10782 found via fuzzing. The regression is 16-bit immediates were forced to fit from an `i32` value into a `u16` for 16-bit multiplication. This meant though that negative numbers failed this conversion which meant that ISLE would panic due to the value not being matched. This fixes the logic to first fit the i32 into an i16 and then cast that to a u16 where the first phase should hit all the constants that are possible in Cranelift. * Add some more tests
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 20, 2025
* x64: Fix encoding of RIP-relative addressing This commit fixes a bug in the new assembler which was surfaced through the changes in #10782 but was a pre-existing issue. Specifically the encoding of a RIP-relative addressing mode required knowing the number of bytes at the end of an instruction but this was accidentally hardcoded to 0. In #10782 `imul` instructions were added where a RIP-relative address mode can be used in conjunction with an immediate which cause the RIP-relative addressing to load from the wrong address. This bug can in theory affect other instructions in the new assembler as well, but auditing the list of instructions it looks like `imul` is the only one that can possibly have an immediate after a RIP-relative addressing mode. That means that prior instructions using the new assembler should not be affected. * Fix testing the assembler crate * Apply suggestions from code review Co-authored-by: Andrew Brown <[email protected]> --------- Co-authored-by: Andrew Brown <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Multiplication on x86 is a bit tricky: there are a few different instruction formats, some gaps, and some implicit registers holding different sets of bits (
rax,rdx). This PR converts all x64 backend multiplications to use the new assembler and, in doing so, adds support for implicit registers: these are operands that need to be tracked during register allocation but may not show up in instruction disassembly. There is more that could be done to refactor these multiplication rules but that could come later.